feat(support): add ApiError carrier and generated per-operation error-factory classes for spec-declared error responses - #169
Conversation
…en regex rule An untrusted-spec `pattern` that is ECMA-valid-but-PCRE-invalid (or syntactically broken, e.g. `(` or a trailing `\`) was embedded verbatim into a Laravel `regex:...` rule with no compile probe. Laravel's preg_match then raises an UNCATCHABLE compile error on every request to that field, a runtime 500/DoS in the consumer's app that still passes `php -l` and the corpus gate. regexRule() now probes the delimited pattern with the existing compilesAsPcre() helper (the same probe closedObjectRule() already runs over patternProperties patterns) before emitting. A pattern that does not compile is dropped, the field keeps its other validation rules, and the skip is surfaced as a build warning via the existing state->warnings channel. The corpus specs carry only valid PCRE patterns, so output is byte-identical for all 135 specs (GenerateCorpusTest, PetstoreDriftTest, and the conformance/golden suites confirm zero drift). Closes #150
The spec is untrusted input. A non-finite float reaches a numeric keyword from JSON (`1e400` decodes to INF) or YAML (`.inf`/`.nan`), and `numberValue()` passed it through unchanged. The emitter then produced a degenerate rule that is syntactically valid PHP and so cleared every quality gate: `max:NAN` rejects EVERY value for the field (an availability bug planted purely by spec input), and `min:INF` / `MultipleOfRule(INF)` are nonsensical. `numberValue()` now applies a single `is_finite()` guard at the one chokepoint that feeds all five numeric keywords (minimum, maximum, multipleOf, exclusiveMinimum, exclusiveMaximum, and transitively the integer-keyword path via intValue). A non-finite float, native or coerced from an overflowing numeric string like "1e400", returns null and lands in the existing graceful-ignored `extra` path, exactly as an absent or non-numeric keyword already does. The keyword is dropped from the typed schema rather than emitted as a broken rule. Corpus specs use only finite numbers, so output is byte-identical for all 135 specs (GenerateCorpusTest confirms zero drift). Closes #151
EnumEmitter inferred an int backing for any unsigned-digit string, then emitted each case literal as `(int) $value`. A non-canonical decimal string was silently corrupted: `"01"` became `case Value1 = 1`, so the spec wire value `"01"` no longer round-tripped (a consumer sending `"01"` never matched the enum), and an enum carrying both `"01"` and `"1"` collapsed both cases to the SAME `1`, emitting two `case ... = 1;` lines: a fatal "Duplicate value in enum" PHP error in the generated app. All of this passed php -l and the corpus gate because the single-value case still produced syntactically valid PHP. A new isIntBackable() helper now requires a string to ALSO round-trip through int unchanged (`(string) (int) $value === $value`) before it counts as int-backable; the unsigned-digit gate is kept so a signed string like `"-1"` stays string-backed exactly as before. A non-canonical value (`"01"`, `"040000"`, `"00"`) falls back to a faithful string backing, preserving the wire value and keeping sibling cases distinct. Corpus enum-class output is byte-identical for all 135 specs (verified with a full before/after diff of every generated enum file): no corpus enum currently relies on the corrupting path. Closes #145
PhpLiteral::numberLiteral returned `(string) $value`, which stringifies a small- or large-magnitude float in scientific notation: `(string) 1e-7` is `"1.0E-7"`, `(string) 1e20` is `"1.0E+20"`. That form is embedded verbatim into generated Laravel rule strings (`min:1.0E-7`, `gt:1.0E-7`, `lt:...`, the MultipleOfRule argument) and into property defaults. In a rule-string parameter the validator reads the literal text and the `E` is not understood as an exponent, so a spec-legal tiny `minimum`/`multipleOf` becomes a broken or wrongly-parsed rule; in a default it is needlessly opaque. numberLiteral now expands any scientific-notation rendering into plain fixed-decimal by shifting the decimal point per the exponent, preserving the exact digits the cast produced (the precision is unchanged, only the format). A non-scientific value is returned untouched, so every normal-range number is byte-identical to before and no corpus output drifts. Closes #148
📝 WalkthroughWalkthroughAdds self-rendering ChangesGenerated API error handling
Enum backing inference
Numeric literal rendering
Regex rule validation
Finite numeric parsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant OperationErrors
participant ApiError
participant Laravel
Controller->>OperationErrors: throw status-specific factory
OperationErrors->>ApiError: construct DTO carrier with HTTP status
ApiError->>Laravel: render JSON response
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Qodana for PHPIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Detected 12 dependenciesThird-party software listThis page lists the third-party software dependencies used in project
Contact Qodana teamContact us at qodana-support@jetbrains.com
|
4864e58 to
6364ef8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Unit/Security/HostileSpecTest.php (1)
227-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover numeric-string overflow and exclusive bounds.
The new
"1e400"coercion guard and theexclusiveMinimum/exclusiveMaximumcall paths remain untested. Add cases confirming they also emit no non-finite constraints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Security/HostileSpecTest.php` around lines 227 - 260, Extend the numeric constraint tests around generateNumberConstraint to cover numeric-string overflow such as "1e400" and the exclusiveMinimum/exclusiveMaximum paths. Assert that non-finite coerced values produce no corresponding exclusive minimum or maximum constraints, including no INF/NAN output, while preserving the existing finite-bound coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Emitter/Server/OperationCollector.php`:
- Around line 1895-1897: Update the response-slot filtering logic in
OperationCollector to record warnings whenever default, 4XX/5XX wildcard, or
other unsupported error responses are skipped, including their specific skip
reasons. Ensure accumulated $skipped diagnostics are emitted before the early
return when $slots is empty, so inline-schema warnings are not suppressed; add
coverage for operations containing only unsupported error slots.
---
Nitpick comments:
In `@tests/Unit/Security/HostileSpecTest.php`:
- Around line 227-260: Extend the numeric constraint tests around
generateNumberConstraint to cover numeric-string overflow such as "1e400" and
the exclusiveMinimum/exclusiveMaximum paths. Assert that non-finite coerced
values produce no corresponding exclusive minimum or maximum constraints,
including no INF/NAN output, while preserving the existing finite-bound
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc0057ec-5732-420d-8325-8490b0f048d0
📒 Files selected for processing (40)
ROADMAP.mdcomposer-require-checker.jsondocs/src/content/docs/guides/runtime-coupling.mdxdocs/src/content/docs/guides/server-scaffold.mdxdocs/src/content/docs/guides/stability.mdxdocs/src/content/docs/guides/validation-errors.mdxdocs/src/content/docs/guides/versioning-policy.mdxe2e/backend/.gitignoree2e/backend/app/Http/Controllers/Api/PetController.phpe2e/e2e-tests/tests/petstore.spec.tse2e/spec/petstore.yamlsrc/Console/GenerationPlanner.phpsrc/Emitter/EnumEmitter.phpsrc/Emitter/ErrorFactorySynthesizer.phpsrc/Emitter/GenerationState.phpsrc/Emitter/ModelGenerator.phpsrc/Emitter/PhpLiteral.phpsrc/Emitter/RulesBuilder.phpsrc/Emitter/Server/OperationCollector.phpsrc/Parser/OpenApiReader.phpsrc/Support/ApiError.phptests/Conformance/ConformanceGoldenTest.phptests/Corpus/GeneratedOutputPhpstanTest.phptests/Corpus/GeneratedOutputPintTest.phptests/Corpus/ReaderCorpusBaselineTest.phptests/Feature/Console/CheckCommandTest.phptests/Feature/Emitter/ApiErrorRoundTripTest.phptests/Feature/Support/ApiErrorRenderTest.phptests/Fixtures/conformance/conformance-3.1.yamltests/Fixtures/corpus-baseline-v0.11.0.jsontests/Fixtures/server/api-error.yamltests/Unit/Console/GenerationPlannerTest.phptests/Unit/Emitter/EnumBackingTest.phptests/Unit/Emitter/ErrorFactorySynthesizerTest.phptests/Unit/Emitter/ModelGeneratorTest.phptests/Unit/Emitter/PhpLiteralTest.phptests/Unit/Emitter/Server/OperationCollectorTest.phptests/Unit/Emitter/ValidationConstraintsTest.phptests/Unit/Security/HostileSpecTest.phptests/Unit/Support/ApiErrorTest.php
6364ef8 to
4dc155b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Emitter/Server/OperationCollector.php (1)
1895-1898: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecord wildcard/default skip reasons to ensure they are warned about.
The PR objective states that
default,4XX, and5XXwildcard responses are "skipped with warnings". However, thecontinuehere bypasses the$skippedarray, meaning these non-concrete statuses will never be included in the warnings, even when an operation successfully gets a factory.🐛 Proposed fix to populate `$skipped`
// v1: only concrete 4xx/5xx codes get a factory method (400-599). if (preg_match('~^[45][0-9][0-9]$~', $status) !== 1) { + $skipped[] = ['status' => $status, 'reason' => 'only concrete 4xx/5xx statuses are supported in this version']; continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Emitter/Server/OperationCollector.php` around lines 1895 - 1898, Update the status-filtering branch in OperationCollector’s response collection logic so non-concrete statuses such as default, 4XX, and 5XX are recorded in the existing $skipped array before continuing. Preserve the concrete 400–599 factory-generation path and ensure successfully collected operations can later warn about these skipped statuses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/Emitter/Server/OperationCollector.php`:
- Around line 1895-1898: Update the status-filtering branch in
OperationCollector’s response collection logic so non-concrete statuses such as
default, 4XX, and 5XX are recorded in the existing $skipped array before
continuing. Preserve the concrete 400–599 factory-generation path and ensure
successfully collected operations can later warn about these skipped statuses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42e31b62-8c03-4984-9f07-bdd7c8e0b2ad
📒 Files selected for processing (31)
ROADMAP.mdcomposer-require-checker.jsondocs/src/content/docs/guides/runtime-coupling.mdxdocs/src/content/docs/guides/server-scaffold.mdxdocs/src/content/docs/guides/stability.mdxdocs/src/content/docs/guides/validation-errors.mdxdocs/src/content/docs/guides/versioning-policy.mdxe2e/backend/.gitignoree2e/backend/app/Http/Controllers/Api/PetController.phpe2e/e2e-tests/tests/petstore.spec.tse2e/spec/petstore.yamlsrc/Console/GenerationPlanner.phpsrc/Emitter/ErrorFactorySynthesizer.phpsrc/Emitter/GenerationState.phpsrc/Emitter/ModelGenerator.phpsrc/Emitter/Server/OperationCollector.phpsrc/Support/ApiError.phptests/Conformance/ConformanceGoldenTest.phptests/Corpus/GeneratedOutputPhpstanTest.phptests/Corpus/GeneratedOutputPintTest.phptests/Corpus/ReaderCorpusBaselineTest.phptests/Feature/Console/CheckCommandTest.phptests/Feature/Emitter/ApiErrorRoundTripTest.phptests/Feature/Support/ApiErrorRenderTest.phptests/Fixtures/conformance/conformance-3.1.yamltests/Fixtures/corpus-baseline-v0.11.0.jsontests/Fixtures/server/api-error.yamltests/Unit/Console/GenerationPlannerTest.phptests/Unit/Emitter/ErrorFactorySynthesizerTest.phptests/Unit/Emitter/Server/OperationCollectorTest.phptests/Unit/Support/ApiErrorTest.php
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/Corpus/GeneratedOutputPhpstanTest.php
- src/Emitter/GenerationState.php
- docs/src/content/docs/guides/stability.mdx
- e2e/backend/.gitignore
- tests/Unit/Support/ApiErrorTest.php
- src/Emitter/ErrorFactorySynthesizer.php
- docs/src/content/docs/guides/server-scaffold.mdx
- src/Console/GenerationPlanner.php
- e2e/backend/app/Http/Controllers/Api/PetController.php
- tests/Corpus/GeneratedOutputPintTest.php
- composer-require-checker.json
- e2e/e2e-tests/tests/petstore.spec.ts
- docs/src/content/docs/guides/validation-errors.mdx
- tests/Fixtures/conformance/conformance-3.1.yaml
- e2e/spec/petstore.yaml
- docs/src/content/docs/guides/versioning-policy.mdx
- tests/Conformance/ConformanceGoldenTest.php
- tests/Unit/Console/GenerationPlannerTest.php
- ROADMAP.md
- tests/Unit/Emitter/Server/OperationCollectorTest.php
- tests/Feature/Support/ApiErrorRenderTest.php
- src/Emitter/ModelGenerator.php
- tests/Corpus/ReaderCorpusBaselineTest.php
- tests/Unit/Emitter/ErrorFactorySynthesizerTest.php
…-factory classes for spec-declared error responses
Generated abstract controller methods are typed to the operation's success DTO
by design, so a concrete controller could not cleanly answer a spec-declared
error status: returning a JsonResponse clashed with the success return type, and
teams hand-rolled the error DTO the generator already emits from the spec.
Add two layers. Controller return types are unchanged: errors are thrown, and a
throw satisfies any return type.
- Support\ApiError: a final, self-rendering throwable inlined into the
consumer's own \Support namespace like RespondsWithStatus, carrying a Data
body plus an HTTP status and rendering via Laravel's render(Request): Response
hook, so no bootstrap/app.php registration is needed. General constructor plus
named-status factories as a documented escape hatch.
- Generated <Operation>Errors factories: one status-keyed static method per
spec-declared named-component object error response, flattening the error
DTO's constructor into named parameters and forwarding into
new ApiError(new <Schema>Data(...), <status>). Placed in the tag-grouped Data
namespace; emission gated by --no-controllers.
throw GetPetByIdErrors::notFound(message: "Pet {$petId} not found.");
The status is written once (in the method name), the error DTO is never
hand-rolled, and an operation can only throw the errors its spec declares. A
shared error schema across statuses yields one method per status, all
forwarding to the same generated DTO.
v1 covers named-component object error schemas; inline-object schemas and the
default/4XX/5XX wildcards are deferred (warn-and-skip), matching the
component-then-inline staging precedent (#110/#76, #116/#129). Extends decision
#11 without a generated renderer: ApiError is a schema-agnostic carrier the
developer fills with an already-generated DTO.
Includes the ReaderCorpusBaselineTest rebaseline: 32 corpus specs gain factory
classes (READER_BASELINE_REBASELINED_168), and aws_iam/sendgrid whose output
also shifted from the bundled uncompilable-pattern fix c670c09
(READER_BASELINE_REBASELINED_RULES_UNCOMPILABLE_PATTERN). Docs, ROADMAP #11
addendum, and a live e2e petstore demo included.
Closes #168
4dc155b to
dac90d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Support/ApiError.php`:
- Around line 129-132: Update ApiError::render to detect when $this->body
implements Responsable and call its toResponse($request) method directly,
returning that response unchanged. Preserve the existing response()->json flow
for bodies that are not Responsable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76a2d567-10e5-40cb-9688-48221e732373
📒 Files selected for processing (34)
ROADMAP.mdcomposer-require-checker.jsondocs/src/content/docs/guides/runtime-coupling.mdxdocs/src/content/docs/guides/server-scaffold.mdxdocs/src/content/docs/guides/stability.mdxdocs/src/content/docs/guides/validation-errors.mdxdocs/src/content/docs/guides/versioning-policy.mdxe2e/backend/.gitignoree2e/backend/app/Http/Controllers/Api/PetController.phpe2e/e2e-tests/tests/petstore.spec.tse2e/spec/petstore.yamlqodana.yamlsrc/Console/GenerationPlanner.phpsrc/Emitter/ErrorFactorySynthesizer.phpsrc/Emitter/GenerationState.phpsrc/Emitter/ModelGenerator.phpsrc/Emitter/RulesBuilder.phpsrc/Emitter/Server/OperationCollector.phpsrc/Support/ApiError.phptests/Conformance/ConformanceGoldenTest.phptests/Corpus/GeneratedOutputPhpstanTest.phptests/Corpus/GeneratedOutputPintTest.phptests/Corpus/ReaderCorpusBaselineTest.phptests/Feature/Console/CheckCommandTest.phptests/Feature/Emitter/ApiErrorRoundTripTest.phptests/Feature/Support/ApiErrorRenderTest.phptests/Fixtures/conformance/conformance-3.1.yamltests/Fixtures/corpus-baseline-v0.11.0.jsontests/Fixtures/server/api-error.yamltests/Unit/Console/GenerationPlannerTest.phptests/Unit/Emitter/ErrorFactorySynthesizerTest.phptests/Unit/Emitter/Server/OperationCollectorTest.phptests/Unit/Security/HostileSpecTest.phptests/Unit/Support/ApiErrorTest.php
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/Corpus/GeneratedOutputPhpstanTest.php
- e2e/backend/app/Http/Controllers/Api/PetController.php
- docs/src/content/docs/guides/versioning-policy.mdx
- composer-require-checker.json
- tests/Unit/Console/GenerationPlannerTest.php
- e2e/backend/.gitignore
- src/Emitter/RulesBuilder.php
- docs/src/content/docs/guides/stability.mdx
- tests/Unit/Support/ApiErrorTest.php
- ROADMAP.md
- tests/Unit/Security/HostileSpecTest.php
- tests/Corpus/GeneratedOutputPintTest.php
- tests/Feature/Support/ApiErrorRenderTest.php
- tests/Conformance/ConformanceGoldenTest.php
- tests/Unit/Emitter/Server/OperationCollectorTest.php
- src/Console/GenerationPlanner.php
- src/Emitter/GenerationState.php
- docs/src/content/docs/guides/validation-errors.mdx
- docs/src/content/docs/guides/server-scaffold.mdx
- src/Emitter/ErrorFactorySynthesizer.php
- src/Emitter/ModelGenerator.php
- src/Emitter/Server/OperationCollector.php
- tests/Corpus/ReaderCorpusBaselineTest.php
- tests/Unit/Emitter/ErrorFactorySynthesizerTest.php
Closes #168.
What & why
From user feedback: a generated abstract controller method's return type is the operation's success DTO by design (error responses are never inspected for typing), so a concrete controller that must answer a spec-declared error status (a 404
ErrorResponse, ...) could not cleanly return an error body, returning aJsonResponseclashes with the success return type ("ExpectedPatientData, foundJsonResponse"), and teams hand-rolled the error DTO the generator already emits from the spec.The change (two layers; controller return types unchanged, you throw)
1.
Support\ApiError, afinal, self-rendering throwable inlined into the consumer's own\Supportnamespace likeRespondsWithStatus. Carries a Data body plus an HTTP status and renders through Laravel'srender(Request): Responsehook, so nobootstrap/app.phpwiring is needed. General constructor plus named-status factories as a documented escape hatch.2. Generated
<Operation>Errorsfactories , one status-keyed static method per spec-declared named-component object error response, flattening the error DTO's constructor into named parameters:The status is written once (in the method name), the error DTO is never hand-rolled, and an operation can only throw the errors its spec declares. A shared error schema across statuses (one
ErrorResponseat 400/401/403/404) yields one method per status, all forwarding to the same generated DTO.Scope (v1)
$refto a generated Data class); concrete 4xx/5xx codes with astatusNNNfallback for uncommon codes.default/4XX/5XXwildcards, matching this repo's component-then-inline staging precedent (Typed Data params for component $ref request bodies #110 then Inline JSON request bodies as typed Data params #76, feat(server): resolve component $ref responses to typed return types #116 then feat(emitter): synthesize typed return from inline (non-$ref) object response schemas #129). TheApiErrorgeneral constructor covers deferred cases.ApiErroris a schema-agnostic carrier the developer fills with an already-generated DTO.Footprint (additive, minor bump)
<Operation>Errorsclasses (32 of the 130 frozen-baseline set). No existing generated file changes , success-path signatures are untouched, so a spec that does not qualify (or--no-controllers) produces byte-identical output.stripe.jsonproduces zero factories (all its errors are declared underdefault, which v1 omits); the earlier planning estimate overstated the footprint, these are the measured numbers.Tests / verification
composer test2281 passed,composer stan(PHPStan max) clean,composer lint(Pint) clean,composer deptrac0 violations,composer test:type100%.statusNNN, shared-schema two-methods, partial-qualifying + skip warnings, discriminated-union skip, readOnly/writeOnly READ-variant), a real generate -> HTTP round-trip (404 body plus an unaffected success path),openapi:checkdrift lockstep for the new files, and the planner--no-controllersveto.PetController::show()toGetPetByIdErrors::notFound(...); a Playwright assertion drives real HTTP ,GET /api/v1/pet/<missing> -> 404 {"message": ...}, control-> 200.ReaderCorpusBaselineTest): the 32 factory-gaining specs are rebaselined underREADER_BASELINE_REBASELINED_168, audited per spec; the baseline pipeline now also hashes the new factory-file bucket so the factories gain drift coverage.Note on bundled commits
This PR also carries 4 pre-existing
mainfixes (floats / enum / parser / rules). One of them ,c670c09(drop uncompilable\uXXXXpatterns) , changed corpus output foraws_iamandsendgridwithout a baseline update, so it is rebaselined here underREADER_BASELINE_REBASELINED_RULES_UNCOMPILABLE_PATTERN.Summary by CodeRabbit
ApiErrorthrowable carrier for status-aware JSON error rendering.regex:rules when patterns can’t compile; tightened enum int/string backing inference to avoid non-canonical decimals.ApiErrorrendering/round-trips, and generator stability.